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

[PATCH v2 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       |   48 +++++++++++++++
 xen/arch/arm/setup.c        |    2 +-
 xen/arch/x86/bzimage.c      |  134 +----------------------------------------
 xen/common/Makefile         |    1 +
 xen/common/gunzip.c         |  138 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/setup.h |    2 +
 xen/include/xen/gunzip.h    |    7 +++
 7 files changed, 198 insertions(+), 134 deletions(-)
 create mode 100644 xen/common/gunzip.c
 create mode 100644 xen/include/xen/gunzip.h

Cheers,

Stefano

[PATCH v2 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>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com

---
Changes in v2:
- the patch has been reworked from scratch
---
 xen/arch/x86/bzimage.c   |  134 +-------------------------------------------
 xen/common/Makefile      |    1 +
 xen/common/gunzip.c      |  138 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/gunzip.h |    7 +++
 4 files changed, 147 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..4061427 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -55,6 +55,7 @@ obj-y += vmap.o
 obj-y += vsprintf.o
 obj-y += wait.o
 obj-y += xmalloc_tlsf.o
+obj-y += gunzip.o
 
 obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
 
diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
new file mode 100644
index 0000000..8da5c2d
--- /dev/null
+++ b/xen/common/gunzip.c
@@ -0,0 +1,138 @@
+#include <xen/config.h>
+#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..7af87e0
--- /dev/null
+++ b/xen/include/xen/gunzip.h
@@ -0,0 +1,7 @@
+#ifndef __XEN_GUNZIP_H
+#define __XEN_GUNZIP_H
+
+__init int gzip_check(char *image, unsigned long image_len);
+__init int perform_gunzip(char *output, char *image, unsigned long image_len);
+
+#endif
-- 
1.7.10.4

[PATCH v2 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>
CC: julien.grall@citrix.com
CC: ian.campbell@citrix.com

---

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       |   48 +++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/setup.c        |    2 +-
 xen/include/asm-arm/setup.h |    2 ++
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index f641b12..e7fbb24 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,43 @@ static int kernel_uimage_probe(struct kernel_info *info,
     return 0;
 }
 
+static __init unsigned long output_length(char *image, unsigned long image_len)
+{
+    return *(uint32_t *)&image[image_len - 4];
+}
+
+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;
+    
+    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);
+
+    output_size = output_length(input, *size);
+    kernel_order_out = get_order_from_bytes(output_size);
+    output = alloc_xenheap_pages(kernel_order_out, 0);
+
+    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;
+    }
 #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 v2 2/2] xen/arm: support gzip compressed kernels

[Julien Grall]

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

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

[Jan Beulich]

>>> On 13.08.15 at 13:21, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -55,6 +55,7 @@ obj-y += vmap.o
>  obj-y += vsprintf.o
>  obj-y += wait.o
>  obj-y += xmalloc_tlsf.o
> +obj-y += gunzip.o
>  
>  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)

Isn't the code you add in gunzip.c all __init / __initdata (or could at least
be)? If so, this should become obj-bin-y += gunzip.o just like is being
done for all the other decompressors.

> --- /dev/null
> +++ b/xen/common/gunzip.c
> @@ -0,0 +1,138 @@
> +#include <xen/config.h>

This should no be included explicitly in new code.

> +#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))

I understand that you're mostly moving code, but I'd appreciate if
you did some formatting adjustments along the way (like removing
the superfluous parentheses here...

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

... or indentation here).

> +}
> +
> +

Just one blank line please.

> +#include "inflate.c"
> +
> +static __init void flush_window(void)

Any reason this can't be placed ahead of the #include, avoiding the
extra declaration earlier on?

> +__init int gzip_check(char *image, unsigned long image_len)

int __init gzip_check(...

> +{
> +    unsigned char magic0, magic1;
> +
> +    if ( image_len < 2 )
> +        return 0;
> +
> +    magic0 = (unsigned char)image[0];
> +    magic1 = (unsigned char)image[1];

Pointless casts?

> +__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;

Any reason output and image can't be declared "unsigned char *"
right away, avoiding such bogus casts? (Same then for
gzip_check().)

> +
> +    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;
> +    }

Many pointless braces.

> --- /dev/null
> +++ b/xen/include/xen/gunzip.h
> @@ -0,0 +1,7 @@
> +#ifndef __XEN_GUNZIP_H
> +#define __XEN_GUNZIP_H
> +
> +__init int gzip_check(char *image, unsigned long image_len);
> +__init int perform_gunzip(char *output, char *image, unsigned long image_len);

No __init on declarations please.

Jan

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

[Stefano Stabellini]

On Thu, 13 Aug 2015, Jan Beulich wrote:
> >>> On 13.08.15 at 13:21, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -55,6 +55,7 @@ obj-y += vmap.o
> >  obj-y += vsprintf.o
> >  obj-y += wait.o
> >  obj-y += xmalloc_tlsf.o
> > +obj-y += gunzip.o
> >  
> >  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> 
> Isn't the code you add in gunzip.c all __init / __initdata (or could at least
> be)? If so, this should become obj-bin-y += gunzip.o just like is being
> done for all the other decompressors.

OK, I'll make the change


> > --- /dev/null
> > +++ b/xen/common/gunzip.c
> > @@ -0,0 +1,138 @@
> > +#include <xen/config.h>
> 
> This should no be included explicitly in new code.

OK
        

> > +#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))
> 
> I understand that you're mostly moving code, but I'd appreciate if
> you did some formatting adjustments along the way (like removing
> the superfluous parentheses here...

I prefer to keep code movement as code movement -- much easier to bisect
or spot regressions. If you have any changes that you really require on
top of the movement, I could carry them on a separate patch on top of
this.


> > +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;
> 
> ... or indentation here).
> 
> > +}
> > +
> > +
> 
> Just one blank line please.
> 
> > +#include "inflate.c"
> > +
> > +static __init void flush_window(void)
> 
> Any reason this can't be placed ahead of the #include, avoiding the
> extra declaration earlier on?
> 
> > +__init int gzip_check(char *image, unsigned long image_len)
> 
> int __init gzip_check(...
> 
> > +{
> > +    unsigned char magic0, magic1;
> > +
> > +    if ( image_len < 2 )
> > +        return 0;
> > +
> > +    magic0 = (unsigned char)image[0];
> > +    magic1 = (unsigned char)image[1];
> 
> Pointless casts?
> 
> > +__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;
> 
> Any reason output and image can't be declared "unsigned char *"
> right away, avoiding such bogus casts? (Same then for
> gzip_check().)

all this code was left untouched by the movement, but FYI simply
changing output and image to unsigned char * would break the compilation
of bzimage.c


> > +
> > +    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;
> > +    }
> 
> Many pointless braces.
> 
> > --- /dev/null
> > +++ b/xen/include/xen/gunzip.h
> > @@ -0,0 +1,7 @@
> > +#ifndef __XEN_GUNZIP_H
> > +#define __XEN_GUNZIP_H
> > +
> > +__init int gzip_check(char *image, unsigned long image_len);
> > +__init int perform_gunzip(char *output, char *image, unsigned long image_len);
> 
> No __init on declarations please.

OK

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

[Jan Beulich]

>>> On 01.09.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 13 Aug 2015, Jan Beulich wrote:
>> >>> On 13.08.15 at 13:21, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/common/Makefile
>> > +++ b/xen/common/Makefile
>> > @@ -55,6 +55,7 @@ obj-y += vmap.o
>> >  obj-y += vsprintf.o
>> >  obj-y += wait.o
>> >  obj-y += xmalloc_tlsf.o
>> > +obj-y += gunzip.o
>> >  
>> >  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
>> 
>> Isn't the code you add in gunzip.c all __init / __initdata (or could at least
>> be)? If so, this should become obj-bin-y += gunzip.o just like is being
>> done for all the other decompressors.
> 
> OK, I'll make the change

Just for the avoidance of doubt or future confusion: I didn't spell
this out correctly, it ought to be "obj-bin-y += gunzip.init.o".

>> > +#define memzero(s, n)   memset((s), 0, (n))
>> 
>> I understand that you're mostly moving code, but I'd appreciate if
>> you did some formatting adjustments along the way (like removing
>> the superfluous parentheses here...
> 
> I prefer to keep code movement as code movement -- much easier to bisect
> or spot regressions. If you have any changes that you really require on
> top of the movement, I could carry them on a separate patch on top of
> this.

Well, okay then (albeit I'm not really happy about the code that
way remaining as crappy as it used to be).

Jan

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

On Tue, 2015-09-01 at 15:57 +0100, Stefano Stabellini wrote:
> 
> > 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.

That probably depends on lots of things, in particular the setting of
CONFIG_SEPARATE_XENHEAP and in any case I think it should be avoided as a
matter of principal.

Also, modules are freed to the domheap not the xenheap. 

Maybe it would be best if this memory was allocated from there?

Ian.

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

[Stefano Stabellini]

On Tue, 1 Sep 2015, Ian Campbell wrote:
> On Tue, 2015-09-01 at 15:57 +0100, Stefano Stabellini wrote:
> > 
> > > 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.
> 
> That probably depends on lots of things, in particular the setting of
> CONFIG_SEPARATE_XENHEAP and in any case I think it should be avoided as a
> matter of principal.
> 
> Also, modules are freed to the domheap not the xenheap. 
> 
> Maybe it would be best if this memory was allocated from there?

Yes, you are right, it would definitely be better.