[PATCH v5 0/2] support gzipped kernels on arm

[PATCH v5 0/2] support gzipped kernels on arm

[Stefano Stabellini]

Hi all,

this patch series introduces support for gzipped kernels, such as the
standard Image.gz format used by Linux on arm64 by default, in Xen on
arm. Without it, Xen cannot load the default kernel shipped by distros,
such as CentOS 7.


Stefano Stabellini (2):
      xen: move perform_gunzip to common
      xen/arm: support gzip compressed kernels

 xen/arch/arm/kernel.c       |   75 +++++++++++++++++++++++
 xen/arch/arm/setup.c        |    2 +-
 xen/arch/x86/bzimage.c      |  134 +-----------------------------------------
 xen/common/Makefile         |    1 +
 xen/common/gunzip.c         |  137 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/setup.h |    2 +
 xen/include/xen/gunzip.h    |    7 +++
 7 files changed, 224 insertions(+), 134 deletions(-)
 create mode 100644 xen/common/gunzip.c
 create mode 100644 xen/include/xen/gunzip.h

Cheers,

Stefano

[PATCH v5 1/2] xen: move perform_gunzip to common

[Stefano Stabellini]

The current gunzip code to decompress the Dom0 kernel is implemented in
inflate.c which is included by bzimage.c.

I am looking to doing the same on ARM64 but there is quite a bit of
boilerplate definitions that I would need to import in order for
inflate.c to work correctly.

Instead of copying/pasting the code from x86/bzimage.c, move those
definitions to a new common file, gunzip.c. Export only perform_gunzip
and gzip_check. Leave output_length where it is.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
CC: andrew.cooper3@citrix.com

---
Changes in v4:
- move gunzip.init.o to its alphabetically correct place

Changes in v3:
- build gunzip.c as gunzip.init.o
- remove #include <xen/config.h>
- remove __init from declarations

Changes in v2:
- the patch has been reworked from scratch
---
 xen/arch/x86/bzimage.c   |  134 +--------------------------------------------
 xen/common/Makefile      |    1 +
 xen/common/gunzip.c      |  137 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/gunzip.h |    7 +++
 4 files changed, 146 insertions(+), 133 deletions(-)
 create mode 100644 xen/common/gunzip.c
 create mode 100644 xen/include/xen/gunzip.h

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index c86c39e..50ebb84 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -4,148 +4,16 @@
 #include <xen/mm.h>
 #include <xen/string.h>
 #include <xen/types.h>
+#include <xen/gunzip.h>
 #include <xen/decompress.h>
 #include <xen/libelf.h>
 #include <asm/bzimage.h>
 
-#define HEAPORDER 3
-
-static unsigned char *__initdata window;
-#define memptr long
-static memptr __initdata free_mem_ptr;
-static memptr __initdata free_mem_end_ptr;
-
-#define WSIZE           0x80000000
-
-static unsigned char *__initdata inbuf;
-static unsigned __initdata insize;
-
-/* Index of next byte to be processed in inbuf: */
-static unsigned __initdata inptr;
-
-/* Bytes in output buffer: */
-static unsigned __initdata outcnt;
-
-#define OF(args)        args
-#define STATIC          static
-
-#define memzero(s, n)   memset((s), 0, (n))
-
-typedef unsigned char   uch;
-typedef unsigned short  ush;
-typedef unsigned long   ulg;
-
-#define INIT            __init
-#define INITDATA        __initdata
-
-#define get_byte()      (inptr < insize ? inbuf[inptr++] : fill_inbuf())
-
-/* Diagnostic functions */
-#ifdef DEBUG
-#  define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
-#  define Trace(x)      do { fprintf x; } while (0)
-#  define Tracev(x)     do { if (verbose) fprintf x ; } while (0)
-#  define Tracevv(x)    do { if (verbose > 1) fprintf x ; } while (0)
-#  define Tracec(c, x)  do { if (verbose && (c)) fprintf x ; } while (0)
-#  define Tracecv(c, x) do { if (verbose > 1 && (c)) fprintf x ; } while (0)
-#else
-#  define Assert(cond, msg)
-#  define Trace(x)
-#  define Tracev(x)
-#  define Tracevv(x)
-#  define Tracec(c, x)
-#  define Tracecv(c, x)
-#endif
-
-static long __initdata bytes_out;
-static void flush_window(void);
-
-static __init void error(char *x)
-{
-    panic("%s", x);
-}
-
-static __init int fill_inbuf(void)
-{
-        error("ran out of input data");
-        return 0;
-}
-
-
-#include "../../common/inflate.c"
-
-static __init void flush_window(void)
-{
-    /*
-     * The window is equal to the output buffer therefore only need to
-     * compute the crc.
-     */
-    unsigned long c = crc;
-    unsigned n;
-    unsigned char *in, ch;
-
-    in = window;
-    for ( n = 0; n < outcnt; n++ )
-    {
-        ch = *in++;
-        c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
-    }
-    crc = c;
-
-    bytes_out += (unsigned long)outcnt;
-    outcnt = 0;
-}
-
 static __init unsigned long output_length(char *image, unsigned long image_len)
 {
     return *(uint32_t *)&image[image_len - 4];
 }
 
-static __init int gzip_check(char *image, unsigned long image_len)
-{
-    unsigned char magic0, magic1;
-
-    if ( image_len < 2 )
-        return 0;
-
-    magic0 = (unsigned char)image[0];
-    magic1 = (unsigned char)image[1];
-
-    return (magic0 == 0x1f) && ((magic1 == 0x8b) || (magic1 == 0x9e));
-}
-
-static __init int perform_gunzip(char *output, char *image, unsigned long image_len)
-{
-    int rc;
-
-    if ( !gzip_check(image, image_len) )
-        return 1;
-
-    window = (unsigned char *)output;
-
-    free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
-    free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
-
-    inbuf = (unsigned char *)image;
-    insize = image_len;
-    inptr = 0;
-
-    makecrc();
-
-    if ( gunzip() < 0 )
-    {
-        rc = -EINVAL;
-    }
-    else
-    {
-        rc = 0;
-    }
-
-    free_xenheap_pages((void *)free_mem_ptr, HEAPORDER);
-
-    return rc;
-}
-
 struct __packed setup_header {
         uint8_t         _pad0[0x1f1];           /* skip uninteresting stuff */
         uint8_t         setup_sects;
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 3fdf931..e681aaa 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -10,6 +10,7 @@ obj-y += event_channel.o
 obj-y += event_fifo.o
 obj-y += grant_table.o
 obj-y += guestcopy.o
+obj-bin-y += gunzip.init.o
 obj-y += irq.o
 obj-y += kernel.o
 obj-y += keyhandler.o
diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
new file mode 100644
index 0000000..41d71ef
--- /dev/null
+++ b/xen/common/gunzip.c
@@ -0,0 +1,137 @@
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+
+#define HEAPORDER 3
+
+static unsigned char *__initdata window;
+#define memptr long
+static memptr __initdata free_mem_ptr;
+static memptr __initdata free_mem_end_ptr;
+
+#define WSIZE           0x80000000
+
+static unsigned char *__initdata inbuf;
+static unsigned __initdata insize;
+
+/* Index of next byte to be processed in inbuf: */
+static unsigned __initdata inptr;
+
+/* Bytes in output buffer: */
+static unsigned __initdata outcnt;
+
+#define OF(args)        args
+#define STATIC          static
+
+#define memzero(s, n)   memset((s), 0, (n))
+
+typedef unsigned char   uch;
+typedef unsigned short  ush;
+typedef unsigned long   ulg;
+
+#define INIT            __init
+#define INITDATA        __initdata
+
+#define get_byte()      (inptr < insize ? inbuf[inptr++] : fill_inbuf())
+
+/* Diagnostic functions */
+#ifdef DEBUG
+#  define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
+#  define Trace(x)      do { fprintf x; } while (0)
+#  define Tracev(x)     do { if (verbose) fprintf x ; } while (0)
+#  define Tracevv(x)    do { if (verbose > 1) fprintf x ; } while (0)
+#  define Tracec(c, x)  do { if (verbose && (c)) fprintf x ; } while (0)
+#  define Tracecv(c, x) do { if (verbose > 1 && (c)) fprintf x ; } while (0)
+#else
+#  define Assert(cond, msg)
+#  define Trace(x)
+#  define Tracev(x)
+#  define Tracevv(x)
+#  define Tracec(c, x)
+#  define Tracecv(c, x)
+#endif
+
+static long __initdata bytes_out;
+static void flush_window(void);
+
+static __init void error(char *x)
+{
+    panic("%s", x);
+}
+
+static __init int fill_inbuf(void)
+{
+        error("ran out of input data");
+        return 0;
+}
+
+
+#include "inflate.c"
+
+static __init void flush_window(void)
+{
+    /*
+     * The window is equal to the output buffer therefore only need to
+     * compute the crc.
+     */
+    unsigned long c = crc;
+    unsigned n;
+    unsigned char *in, ch;
+
+    in = window;
+    for ( n = 0; n < outcnt; n++ )
+    {
+        ch = *in++;
+        c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
+    }
+    crc = c;
+
+    bytes_out += (unsigned long)outcnt;
+    outcnt = 0;
+}
+
+__init int gzip_check(char *image, unsigned long image_len)
+{
+    unsigned char magic0, magic1;
+
+    if ( image_len < 2 )
+        return 0;
+
+    magic0 = (unsigned char)image[0];
+    magic1 = (unsigned char)image[1];
+
+    return (magic0 == 0x1f) && ((magic1 == 0x8b) || (magic1 == 0x9e));
+}
+
+__init int perform_gunzip(char *output, char *image, unsigned long image_len)
+{
+    int rc;
+
+    if ( !gzip_check(image, image_len) )
+        return 1;
+
+    window = (unsigned char *)output;
+
+    free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
+    free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
+
+    inbuf = (unsigned char *)image;
+    insize = image_len;
+    inptr = 0;
+
+    makecrc();
+
+    if ( gunzip() < 0 )
+    {
+        rc = -EINVAL;
+    }
+    else
+    {
+        rc = 0;
+    }
+
+    free_xenheap_pages((void *)free_mem_ptr, HEAPORDER);
+
+    return rc;
+}
diff --git a/xen/include/xen/gunzip.h b/xen/include/xen/gunzip.h
new file mode 100644
index 0000000..8058331
--- /dev/null
+++ b/xen/include/xen/gunzip.h
@@ -0,0 +1,7 @@
+#ifndef __XEN_GUNZIP_H
+#define __XEN_GUNZIP_H
+
+int gzip_check(char *image, unsigned long image_len);
+int perform_gunzip(char *output, char *image, unsigned long image_len);
+
+#endif
-- 
1.7.10.4

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

[Stefano Stabellini]

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>
Reviewed-by: Julien Grall <julien.grall@citrix.com>
CC: ian.campbell@citrix.com

---

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);
+    *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));
+
+    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)
+        return rc;
+    else if (!rc)
+    {
+        /* 
+         * 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;
+    }
+
 #ifdef CONFIG_ARM_64
     rc = kernel_zimage64_probe(info, start, size);
     if (rc < 0)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6626eba..109c71c 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -165,7 +165,7 @@ static void __init processor_id(void)
     processor_setup();
 }
 
-static void dt_unreserved_regions(paddr_t s, paddr_t e,
+void dt_unreserved_regions(paddr_t s, paddr_t e,
                                   void (*cb)(paddr_t, paddr_t), int first)
 {
     int i, nr = fdt_num_mem_rsv(device_tree_flattened);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 81bb3da..30ac53b 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -54,6 +54,8 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
 int construct_dom0(struct domain *d);
 
 void discard_initial_modules(void);
+void dt_unreserved_regions(paddr_t s, paddr_t e,
+                           void (*cb)(paddr_t, paddr_t), int first);
 
 size_t __init boot_fdt_info(const void *fdt, paddr_t paddr);
 const char __init *boot_fdt_cmdline(const void *fdt);
-- 
1.7.10.4

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

[Ian Campbell]

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@eu.citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com>
> CC: ian.campbell@citrix.com
> 
> ---
> 
> 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.

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

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

[Jan Beulich]

>>> On 08.09.15 at 15:35, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-09-07 at 15:25 +0100, Stefano Stabellini wrote:
>> +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.

Indeed, but then the page_to_virt() a few lines up isn't valid either.

Jan

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

[Stefano Stabellini]

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@eu.citrix.com>
> > Reviewed-by: Julien Grall <julien.grall@citrix.com>
> > CC: ian.campbell@citrix.com
> > 
> > ---
> > 
> > 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.
> 

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

[Ian Campbell]

On Fri, 2015-09-11 at 17:20 +0100, Stefano Stabellini wrote:
> 
> > > +
> > > +    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.

We aren't making any use of that information though, and nor is there any
real reason to do so in the caller.

If you care are about the distinction between "not compressed" and
"successfully decompressed" then logging "Successfully decompressed kernel"
at the end of kernel_decompress would be far more useful than returning
some errno instead of 0.

The more important bit to me though is to move the update of mod
->{size,start} and the freeing of the old memory into the decompress
function, such that it is called and returns in a completely consistent
state, rather than returning in an inconsistent state and requiring the
caller to fix it up.

Ian.