[PATCH 0/2] support compressed kernels on ARM64

[PATCH 0/2] support compressed kernels on ARM64

[Stefano Stabellini]

Hi all,

this patch series introduces support for compressed kernels, such as
the standard Image.gz format used by Linux on arm64 by default, in Xen.
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 compressed kernels

 xen/arch/arm/kernel.c           |   36 ++++++++++
 xen/arch/x86/bzimage.c          |  142 +--------------------------------------
 xen/common/Makefile             |    2 +-
 xen/common/decompress.c         |    2 +
 xen/common/gunzip.c             |  141 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/byteorder.h |    2 +
 xen/include/xen/decompress.h    |   11 ++-
 7 files changed, 193 insertions(+), 143 deletions(-)
 create mode 100644 xen/common/gunzip.c

Cheers,

Stefano

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

[Stefano Stabellini]

The current gunzip code to uncompress 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 and integrate the gunzip
uncompressing route into the decompress function.

Only support the simplest calling case (which actually is the only one
in use), return -EOPNOTSUPP if the fill or flush functions are passed as
arguments.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: JBeulich@suse.com
CC: andrew.cooper3@citrix.com
---
 xen/arch/x86/bzimage.c       |  142 +-----------------------------------------
 xen/common/Makefile          |    2 +-
 xen/common/decompress.c      |    2 +
 xen/common/gunzip.c          |  141 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/decompress.h |   11 +++-
 5 files changed, 155 insertions(+), 143 deletions(-)
 create mode 100644 xen/common/gunzip.c

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index c86c39e..976225c 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -8,144 +8,6 @@
 #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;
@@ -258,9 +120,7 @@ int __init bzimage_parse(char *image_base, char **image_start, unsigned long *im
 
     output_len = output_length(*image_start, orig_image_len);
 
-    if ( (err = perform_gunzip(image_base, *image_start, orig_image_len)) > 0 )
-        err = decompress(*image_start, orig_image_len, image_base);
-
+    err = decompress(*image_start, orig_image_len, image_base);
     if ( !err )
     {
         *image_start = image_base;
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 3fdf931..0a4d4fa 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -56,7 +56,7 @@ obj-y += vsprintf.o
 obj-y += wait.o
 obj-y += xmalloc_tlsf.o
 
-obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
+obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
 
 obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
diff --git a/xen/common/decompress.c b/xen/common/decompress.c
index 5f86af9..67b48ec 100644
--- a/xen/common/decompress.c
+++ b/xen/common/decompress.c
@@ -16,6 +16,8 @@ int __init decompress(void *inbuf, unsigned int len, void *outbuf)
          (!memcmp(inbuf, "\037\213", 2) || !memcmp(inbuf, "\037\236", 2)) )
         return gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);
 #endif
+    if (gzip_check(inbuf, len))
+        return perform_gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);
 
     if ( len >= 3 && !memcmp(inbuf, "\x42\x5a\x68", 3) )
         return bunzip2(inbuf, len, NULL, NULL, outbuf, NULL, error);
diff --git a/xen/common/gunzip.c b/xen/common/gunzip.c
new file mode 100644
index 0000000..cb3e0c6
--- /dev/null
+++ b/xen/common/gunzip.c
@@ -0,0 +1,141 @@
+#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 __initdata void (*error)(const char *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;
+}
+
+int INIT 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));
+}
+
+int INIT perform_gunzip(unsigned char *image, unsigned int image_len,
+			int(*fill)(void*, unsigned int),
+			int(*flush)(void*, unsigned int),
+			unsigned char *output,
+			unsigned int *pos,
+			void(*error_fn)(const char *x))
+{
+    int rc;
+
+    /* currently not supported */
+    if (fill != NULL || flush != NULL)
+        return -EOPNOTSUPP;
+
+    error = error_fn;
+    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/decompress.h b/xen/include/xen/decompress.h
index b2955fa..248f0fb 100644
--- a/xen/include/xen/decompress.h
+++ b/xen/include/xen/decompress.h
@@ -1,6 +1,8 @@
 #ifndef __XEN_GENERIC_H
 #define __XEN_GENERIC_H
 
+#include <xen/types.h>
+
 typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
                           int (*fill)(void*, unsigned int),
                           int (*flush)(void*, unsigned int),
@@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
  * dependent).
  */
 
-decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
+decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
 
 int decompress(void *inbuf, unsigned int len, void *outbuf);
 
+static inline unsigned long output_length(char *image, unsigned long image_len)
+{
+    return *(uint32_t *)&image[image_len - 4];
+}
+int gzip_check(char *image, unsigned long image_len);
+
+
 #endif
-- 
1.7.10.4

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

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: julien.grall@citrix.com
CC: ian.campbell@citrix.com
---
 xen/arch/arm/kernel.c           |   36 ++++++++++++++++++++++++++++++++++++
 xen/common/Makefile             |    2 +-
 xen/include/asm-arm/byteorder.h |    2 ++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index f641b12..ca50cdd 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/decompress.h>
+#include <xen/vmap.h>
 
 #include "kernel.h"
 
@@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info *info,
 
     return 0;
 }
+
+static int kernel_zimage64_compressed_probe(struct kernel_info *info,
+                                 paddr_t addr, paddr_t size)
+{
+    char *output, *input;
+    unsigned char magic[2];
+    int rc;
+    unsigned kernel_order_in;
+    unsigned kernel_order_out;
+    paddr_t output_size;
+    
+    copy_from_paddr(magic, addr, sizeof(magic));
+
+    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))
+        return -EINVAL;
+
+    kernel_order_in = get_order_from_bytes(size);
+    input = (char *)ioremap_cache(addr, size);
+
+    output_size = output_length(input, size);
+    kernel_order_out = get_order_from_bytes(output_size);
+    output = (char *)alloc_xenheap_pages(kernel_order_out, 0);
+
+    rc = decompress(input, size, output);
+    clean_dcache_va_range(output, output_size);
+    iounmap(input);
+
+    if (rc != 0)
+        return rc;
+
+    return kernel_zimage64_probe(info, virt_to_maddr(output), output_size);
+}
 #endif
 
 /*
@@ -466,6 +500,8 @@ int kernel_probe(struct kernel_info *info)
 #ifdef CONFIG_ARM_64
     rc = kernel_zimage64_probe(info, start, size);
     if (rc < 0)
+        rc = kernel_zimage64_compressed_probe(info, start, size);
+    if (rc < 0)
 #endif
         rc = kernel_uimage_probe(info, start, size);
     if (rc < 0)
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 0a4d4fa..a8aefc6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -56,7 +56,7 @@ obj-y += vsprintf.o
 obj-y += wait.o
 obj-y += xmalloc_tlsf.o
 
-obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
+obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
 
 obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm-arm/byteorder.h
index 9c712c4..3b7feda 100644
--- a/xen/include/asm-arm/byteorder.h
+++ b/xen/include/asm-arm/byteorder.h
@@ -5,6 +5,8 @@
 
 #include <xen/byteorder/little_endian.h>
 
+#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+
 #endif /* __ASM_ARM_BYTEORDER_H__ */
 /*
  * Local variables:
-- 
1.7.10.4

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

[Ian Campbell]

On Wed, 2015-08-12 at 15:47 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: julien.grall@citrix.com
> CC: ian.campbell@citrix.com
> ---
>  xen/arch/arm/kernel.c           |   36 
> ++++++++++++++++++++++++++++++++++++
>  xen/common/Makefile             |    2 +-
>  xen/include/asm-arm/byteorder.h |    2 ++
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f641b12..ca50cdd 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/decompress.h>
> +#include <xen/vmap.h>
>  
>  #include "kernel.h"
>  
> @@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info 
> *info,
>  
>      return 0;
>  }
> +
> +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
> +                                 paddr_t addr, paddr_t size)
> +{
> +    char *output, *input;
> +    unsigned char magic[2];
> +    int rc;
> +    unsigned kernel_order_in;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    
> +    copy_from_paddr(magic, addr, sizeof(magic));
> +
> +    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))
> +        return -EINVAL;

This is an open coded check_gzip. I think you could call that function on
magic?

> +
> +    kernel_order_in = get_order_from_bytes(size);
> +    input = (char *)ioremap_cache(addr, size);

I don't think you need to cast this, do you? It's a void * already.

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

Likewise.

Where is this buffer freed?

When I said IRL we recover the kernel memory I meant the thing in the boot
modules list. You might be able to get away with flipping the boot module
over to this, but that would have ordering constraints which I didn't
check, it'll probably get subtle fast.

> +
> +    rc = decompress(input, size, output);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    if (rc != 0)
> +        return rc;
> +
> +    return kernel_zimage64_probe(info, virt_to_maddr(output), output_size);



> +}
>  #endif
>  
>  /*
> @@ -466,6 +500,8 @@ int kernel_probe(struct kernel_info *info)
>  #ifdef CONFIG_ARM_64
>      rc = kernel_zimage64_probe(info, start, size);
>      if (rc < 0)
> +        rc = kernel_zimage64_compressed_probe(info, start, size);

I don't see a reason not to support compressed 32 bit kernels too. All it
would take would be to try and uncompress the buffer first before falling
through to the various probe routines, instead of chaining a probe into the
decompressor.

Probably the easiest way to solve this and the buffer allocation issue
above would be to always either copy or decompress the original kernel into
a buffer and then change all the probe function to use a virtual address
instead of an maddr (which might have tricky cache interactions since the
mapping still exists).

> +    if (rc < 0)
>  #endif
>          rc = kernel_uimage_probe(info, start, size);
>      if (rc < 0)
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 0a4d4fa..a8aefc6 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -56,7 +56,7 @@ obj-y += vsprintf.o
>  obj-y += wait.o
>  obj-y += xmalloc_tlsf.o
>  
> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)

I don't think we need/want earlycpio support on ARM (not yet anyway).

>  
>  obj-$(perfc)       += perfc.o
>  obj-$(crash_debug) += gdbstub.o
> diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm
> -arm/byteorder.h
> index 9c712c4..3b7feda 100644
> --- a/xen/include/asm-arm/byteorder.h
> +++ b/xen/include/asm-arm/byteorder.h
> @@ -5,6 +5,8 @@
>  
>  #include <xen/byteorder/little_endian.h>
>  
> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS

While CONFIG_HAVE_UNALIGNED_ACCESS might be true on arm64 it may not be the
case that it is efficient. Also I'm not sure about arm32 at all.


> +
>  #endif /* __ASM_ARM_BYTEORDER_H__ */
>  /*
>   * Local variables:

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

[Julien Grall]

Hi Stefano,

I'm sure you don't support all kind of compressed kernels. Can you
example in the commit message which one you are supporting?

On 12/08/15 15:47, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: julien.grall@citrix.com
> CC: ian.campbell@citrix.com
> ---
>  xen/arch/arm/kernel.c           |   36 ++++++++++++++++++++++++++++++++++++
>  xen/common/Makefile             |    2 +-
>  xen/include/asm-arm/byteorder.h |    2 ++
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f641b12..ca50cdd 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/decompress.h>
> +#include <xen/vmap.h>
>  
>  #include "kernel.h"
>  
> @@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info *info,
>  
>      return 0;
>  }
> +
> +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
> +                                 paddr_t addr, paddr_t size)
> +{
> +    char *output, *input;
> +    unsigned char magic[2];
> +    int rc;
> +    unsigned kernel_order_in;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    
> +    copy_from_paddr(magic, addr, sizeof(magic));
> +
> +    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))

Please add a comment to explain what this magic means.

Maybe you want to use gzip_check?

> +        return -EINVAL;
> +
> +    kernel_order_in = get_order_from_bytes(size);
> +    input = (char *)ioremap_cache(addr, size);

Cast not necessary. And please check that we effectively were able to
map the kernel.

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

Ditto.

> +
> +    rc = decompress(input, size, output);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    if (rc != 0)
> +        return rc;
> +
> +    return kernel_zimage64_probe(info, virt_to_maddr(output), output_size);
> +}
>  #endif
>  
>  /*
> @@ -466,6 +500,8 @@ int kernel_probe(struct kernel_info *info)
>  #ifdef CONFIG_ARM_64
>      rc = kernel_zimage64_probe(info, start, size);
>      if (rc < 0)
> +        rc = kernel_zimage64_compressed_probe(info, start, size);
> +    if (rc < 0)
>  #endif
>          rc = kernel_uimage_probe(info, start, size);
>      if (rc < 0)

Regards,


-- 
Julien Grall

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

[Jan Beulich]

>>> On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/decompress.c
> +++ b/xen/common/decompress.c
> @@ -16,6 +16,8 @@ int __init decompress(void *inbuf, unsigned int len, void *outbuf)
>           (!memcmp(inbuf, "\037\213", 2) || !memcmp(inbuf, "\037\236", 2)) )
>          return gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);
>  #endif
> +    if (gzip_check(inbuf, len))
> +        return perform_gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);

Afaict it would be appropriate for this to replace the #if 0 block the
end of which is seen here as context.

> @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
>   * dependent).
>   */
>  
> -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
> +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
>  
>  int decompress(void *inbuf, unsigned int len, void *outbuf);
>  
> +static inline unsigned long output_length(char *image, unsigned long image_len)

Neither of the callers gets moved out of bzimage.c - why does this
function need to move? And if it's unavoidable I'd suggest naming it
with a meaningful prefix and using the opportunity to constify the
first parameter. And while unlikely for it to not get inlined, retaining
the original __init would seem necessary to avoid build failures which
would result if this didn't get inlined.

Jan

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

[Julien Grall]

On 12/08/15 16:03, Ian Campbell wrote:
> On Wed, 2015-08-12 at 15:47 +0100, Stefano Stabellini wrote:
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> CC: julien.grall@citrix.com
>> CC: ian.campbell@citrix.com
>> ---
>>  xen/arch/arm/kernel.c           |   36 
>> ++++++++++++++++++++++++++++++++++++
>>  xen/common/Makefile             |    2 +-
>>  xen/include/asm-arm/byteorder.h |    2 ++
>>  3 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
>> index f641b12..ca50cdd 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/decompress.h>
>> +#include <xen/vmap.h>
>>  
>>  #include "kernel.h"
>>  
>> @@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info 
>> *info,
>>  
>>      return 0;
>>  }
>> +
>> +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
>> +                                 paddr_t addr, paddr_t size)
>> +{
>> +    char *output, *input;
>> +    unsigned char magic[2];
>> +    int rc;
>> +    unsigned kernel_order_in;
>> +    unsigned kernel_order_out;
>> +    paddr_t output_size;
>> +    
>> +    copy_from_paddr(magic, addr, sizeof(magic));
>> +
>> +    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))
>> +        return -EINVAL;
> 
> This is an open coded check_gzip. I think you could call that function on
> magic?
> 
>> +
>> +    kernel_order_in = get_order_from_bytes(size);
>> +    input = (char *)ioremap_cache(addr, size);
> 
> I don't think you need to cast this, do you? It's a void * already.
> 
>> +
>> +    output_size = output_length(input, size);
>> +    kernel_order_out = get_order_from_bytes(output_size);
>> +    output = (char *)alloc_xenheap_pages(kernel_order_out, 0);
> 
> Likewise.
> 
> Where is this buffer freed?
> 
> When I said IRL we recover the kernel memory I meant the thing in the boot
> modules list. You might be able to get away with flipping the boot module
> over to this, but that would have ordering constraints which I didn't
> check, it'll probably get subtle fast.
> 
>> +
>> +    rc = decompress(input, size, output);
>> +    clean_dcache_va_range(output, output_size);
>> +    iounmap(input);
>> +
>> +    if (rc != 0)
>> +        return rc;
>> +
>> +    return kernel_zimage64_probe(info, virt_to_maddr(output), output_size);
> 
> 
> 
>> +}
>>  #endif
>>  
>>  /*
>> @@ -466,6 +500,8 @@ int kernel_probe(struct kernel_info *info)
>>  #ifdef CONFIG_ARM_64
>>      rc = kernel_zimage64_probe(info, start, size);
>>      if (rc < 0)
>> +        rc = kernel_zimage64_compressed_probe(info, start, size);
> 
> I don't see a reason not to support compressed 32 bit kernels too. All it
> would take would be to try and uncompress the buffer first before falling
> through to the various probe routines, instead of chaining a probe into the
> decompressor.
> 
> Probably the easiest way to solve this and the buffer allocation issue
> above would be to always either copy or decompress the original kernel into
> a buffer and then change all the probe function to use a virtual address
> instead of an maddr (which might have tricky cache interactions since the
> mapping still exists).
> 
>> +    if (rc < 0)
>>  #endif
>>          rc = kernel_uimage_probe(info, start, size);
>>      if (rc < 0)
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index 0a4d4fa..a8aefc6 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -56,7 +56,7 @@ obj-y += vsprintf.o
>>  obj-y += wait.o
>>  obj-y += xmalloc_tlsf.o
>>  
>> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
>> +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> 
> I don't think we need/want earlycpio support on ARM (not yet anyway).
> 
>>  
>>  obj-$(perfc)       += perfc.o
>>  obj-$(crash_debug) += gdbstub.o
>> diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm
>> -arm/byteorder.h
>> index 9c712c4..3b7feda 100644
>> --- a/xen/include/asm-arm/byteorder.h
>> +++ b/xen/include/asm-arm/byteorder.h
>> @@ -5,6 +5,8 @@
>>  
>>  #include <xen/byteorder/little_endian.h>
>>  
>> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> 
> While CONFIG_HAVE_UNALIGNED_ACCESS might be true on arm64 it may not be the
> case that it is efficient. Also I'm not sure about arm32 at all.

ARM32 has alignment checking enabled. So any unaligned access would
result to a data abort.

FWIW, on ARM64 the alignment trap has been disabled because mem*
primitives are relying on the hardware handling misalignment:

58bbe7d71239db508c30099bf7b6db7c458f3336 "  xen: arm64: disable
alignment traps
"

IIRC, the unaligned access on ARM processor tend to be slow. I
remembered to read an article about it a couple of years ago.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 12 Aug 2015, Ian Campbell wrote:
> On Wed, 2015-08-12 at 15:47 +0100, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: julien.grall@citrix.com
> > CC: ian.campbell@citrix.com
> > ---
> >  xen/arch/arm/kernel.c           |   36 
> > ++++++++++++++++++++++++++++++++++++
> >  xen/common/Makefile             |    2 +-
> >  xen/include/asm-arm/byteorder.h |    2 ++
> >  3 files changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index f641b12..ca50cdd 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/decompress.h>
> > +#include <xen/vmap.h>
> >  
> >  #include "kernel.h"
> >  
> > @@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info 
> > *info,
> >  
> >      return 0;
> >  }
> > +
> > +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
> > +                                 paddr_t addr, paddr_t size)
> > +{
> > +    char *output, *input;
> > +    unsigned char magic[2];
> > +    int rc;
> > +    unsigned kernel_order_in;
> > +    unsigned kernel_order_out;
> > +    paddr_t output_size;
> > +    
> > +    copy_from_paddr(magic, addr, sizeof(magic));
> > +
> > +    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))
> > +        return -EINVAL;
> 
> This is an open coded check_gzip. I think you could call that function on
> magic?
> 
> > +
> > +    kernel_order_in = get_order_from_bytes(size);
> > +    input = (char *)ioremap_cache(addr, size);
> 
> I don't think you need to cast this, do you? It's a void * already.
> 
> > +
> > +    output_size = output_length(input, size);
> > +    kernel_order_out = get_order_from_bytes(output_size);
> > +    output = (char *)alloc_xenheap_pages(kernel_order_out, 0);
> 
> Likewise.
> 
> Where is this buffer freed?
> 
> When I said IRL we recover the kernel memory I meant the thing in the boot
> modules list. You might be able to get away with flipping the boot module
> over to this, but that would have ordering constraints which I didn't
> check, it'll probably get subtle fast.
> 
> > +
> > +    rc = decompress(input, size, output);
> > +    clean_dcache_va_range(output, output_size);
> > +    iounmap(input);
> > +
> > +    if (rc != 0)
> > +        return rc;
> > +
> > +    return kernel_zimage64_probe(info, virt_to_maddr(output), output_size);
> 
> 
> 
> > +}
> >  #endif
> >  
> >  /*
> > @@ -466,6 +500,8 @@ int kernel_probe(struct kernel_info *info)
> >  #ifdef CONFIG_ARM_64
> >      rc = kernel_zimage64_probe(info, start, size);
> >      if (rc < 0)
> > +        rc = kernel_zimage64_compressed_probe(info, start, size);
> 
> I don't see a reason not to support compressed 32 bit kernels too. All it
> would take would be to try and uncompress the buffer first before falling
> through to the various probe routines, instead of chaining a probe into the
> decompressor.
> 
> Probably the easiest way to solve this and the buffer allocation issue
> above would be to always either copy or decompress the original kernel into
> a buffer and then change all the probe function to use a virtual address
> instead of an maddr (which might have tricky cache interactions since the
> mapping still exists).

I'll give it a look


> > +    if (rc < 0)
> >  #endif
> >          rc = kernel_uimage_probe(info, start, size);
> >      if (rc < 0)
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index 0a4d4fa..a8aefc6 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -56,7 +56,7 @@ obj-y += vsprintf.o
> >  obj-y += wait.o
> >  obj-y += xmalloc_tlsf.o
> >  
> > -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> > +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> 
> I don't think we need/want earlycpio support on ARM (not yet anyway).

Unfortunately it is not possible to only compile some and not all
because decompress.c makes use of them all.

  
> >  obj-$(perfc)       += perfc.o
> >  obj-$(crash_debug) += gdbstub.o
> > diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm
> > -arm/byteorder.h
> > index 9c712c4..3b7feda 100644
> > --- a/xen/include/asm-arm/byteorder.h
> > +++ b/xen/include/asm-arm/byteorder.h
> > @@ -5,6 +5,8 @@
> >  
> >  #include <xen/byteorder/little_endian.h>
> >  
> > +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> 
> While CONFIG_HAVE_UNALIGNED_ACCESS might be true on arm64 it may not be the
> case that it is efficient. Also I'm not sure about arm32 at all.

It is true on arm archs >= v8, so we should be fine. See:

http://lwn.net/Articles/540022/

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

[Julien Grall]

On 12/08/15 16:22, Stefano Stabellini wrote:
>>>  obj-$(perfc)       += perfc.o
>>>  obj-$(crash_debug) += gdbstub.o
>>> diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm
>>> -arm/byteorder.h
>>> index 9c712c4..3b7feda 100644
>>> --- a/xen/include/asm-arm/byteorder.h
>>> +++ b/xen/include/asm-arm/byteorder.h
>>> @@ -5,6 +5,8 @@
>>>  
>>>  #include <xen/byteorder/little_endian.h>
>>>  
>>> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
>>
>> While CONFIG_HAVE_UNALIGNED_ACCESS might be true on arm64 it may not be the
>> case that it is efficient. Also I'm not sure about arm32 at all.
> 
> It is true on arm archs >= v8, so we should be fine. See:

I guess you meant v6?

Although, unaligned access are not allowed in Xen on ARM32. See
arm32/head.S:353

Given that the uncompress code is only used for decompressing DOM0
kernel, I don't think it's worth to enable unaligned access.

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 12.08.15 at 17:22, <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 12 Aug 2015, Ian Campbell wrote:
>> On Wed, 2015-08-12 at 15:47 +0100, Stefano Stabellini wrote:
>> > --- a/xen/common/Makefile
>> > +++ b/xen/common/Makefile
>> > @@ -56,7 +56,7 @@ obj-y += vsprintf.o
>> >  obj-y += wait.o
>> >  obj-y += xmalloc_tlsf.o
>> >  
>> > -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
>> > +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
>> 
>> I don't think we need/want earlycpio support on ARM (not yet anyway).
> 
> Unfortunately it is not possible to only compile some and not all
> because decompress.c makes use of them all.

Mind pointing out the reference into earlycpio.c?

Jan

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

[Stefano Stabellini]

On Wed, 12 Aug 2015, Jan Beulich wrote:
> >>> On 12.08.15 at 17:22, <stefano.stabellini@eu.citrix.com> wrote:
> > On Wed, 12 Aug 2015, Ian Campbell wrote:
> >> On Wed, 2015-08-12 at 15:47 +0100, Stefano Stabellini wrote:
> >> > --- a/xen/common/Makefile
> >> > +++ b/xen/common/Makefile
> >> > @@ -56,7 +56,7 @@ obj-y += vsprintf.o
> >> >  obj-y += wait.o
> >> >  obj-y += xmalloc_tlsf.o
> >> >  
> >> > -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> >> > +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
> >> 
> >> I don't think we need/want earlycpio support on ARM (not yet anyway).
> > 
> > Unfortunately it is not possible to only compile some and not all
> > because decompress.c makes use of them all.
> 
> Mind pointing out the reference into earlycpio.c?

Sorry, all but earlycpio.c :-/
I'll make the change

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

[Ian Campbell]

On Wed, 2015-08-12 at 16:22 +0100, Stefano Stabellini wrote:
> -obj-bin-$(CONFIG_X86) += $(foreach n,decompress gunzip bunzip2 unxz
> > > unlzma unlzo unlz4 earlycpio,$(n).init.o)
> > > +obj-bin-y += $(foreach n,decompress gunzip bunzip2 unxz unlzma unlzo 
> > > unlz4 earlycpio,$(n).init.o)
> > 
> > I don't think we need/want earlycpio support on ARM (not yet anyway).
> 
> Unfortunately it is not possible to only compile some and not all
> because decompress.c makes use of them all.


earlycpio isn't a decompression algorithm though, it just happens to be
bundled into the same obj-bin. I don't see any sign of cpio support in
decompress.c (since that wouldn't make sense).

> 
> > >  obj-$(perfc)       += perfc.o
> > >  obj-$(crash_debug) += gdbstub.o
> > > diff --git a/xen/include/asm-arm/byteorder.h b/xen/include/asm
> > > -arm/byteorder.h
> > > index 9c712c4..3b7feda 100644
> > > --- a/xen/include/asm-arm/byteorder.h
> > > +++ b/xen/include/asm-arm/byteorder.h
> > > @@ -5,6 +5,8 @@
> > >  
> > >  #include <xen/byteorder/little_endian.h>
> > >  
> > > +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> > 
> > While CONFIG_HAVE_UNALIGNED_ACCESS might be true on arm64 it may not be 
> > the
> > case that it is efficient. Also I'm not sure about arm32 at all.
> 
> It is true on arm archs >= v8,

I'm not sure about that.

>  so we should be fine. See:
> 
> http://lwn.net/Articles/540022/

Even this says it is not always the best option, but this is not fast path
code so never mind.

Also if it isn't true for armv7 you shouldn't set it unconditionally, it
needs either an ifdef or for arm{32,64}/byteorder.h to have it.

On x86 this #define is in config.h, please lets be consistent. Also that
makes more sense anyway since alignment != byteorder.

Ian.

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

[Stefano Stabellini]

On Wed, 12 Aug 2015, Jan Beulich wrote:
> >>> On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/decompress.c
> > +++ b/xen/common/decompress.c
> > @@ -16,6 +16,8 @@ int __init decompress(void *inbuf, unsigned int len, void *outbuf)
> >           (!memcmp(inbuf, "\037\213", 2) || !memcmp(inbuf, "\037\236", 2)) )
> >          return gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);
> >  #endif
> > +    if (gzip_check(inbuf, len))
> > +        return perform_gunzip(inbuf, len, NULL, NULL, outbuf, NULL, error);
> 
> Afaict it would be appropriate for this to replace the #if 0 block the
> end of which is seen here as context.

Yes, I'll make the change.


> > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
> >   * dependent).
> >   */
> >  
> > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
> > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
> >  
> >  int decompress(void *inbuf, unsigned int len, void *outbuf);
> >  
> > +static inline unsigned long output_length(char *image, unsigned long image_len)
> 
> Neither of the callers gets moved out of bzimage.c - why does this
> function need to move?

We'll use it on arm.


> And if it's unavoidable I'd suggest naming it
> with a meaningful prefix and using the opportunity to constify the
> first parameter. And while unlikely for it to not get inlined, retaining
> the original __init would seem necessary to avoid build failures which
> would result if this didn't get inlined.

I'll do.

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

[Andrew Cooper]

On 12/08/15 15:47, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: julien.grall@citrix.com
> CC: ian.campbell@citrix.com
> ---
>  xen/arch/arm/kernel.c           |   36 ++++++++++++++++++++++++++++++++++++
>  xen/common/Makefile             |    2 +-
>  xen/include/asm-arm/byteorder.h |    2 ++
>  3 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index f641b12..ca50cdd 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/decompress.h>
> +#include <xen/vmap.h>
>  
>  #include "kernel.h"
>  
> @@ -310,6 +312,38 @@ static int kernel_zimage64_probe(struct kernel_info *info,
>  
>      return 0;
>  }
> +
> +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
> +                                 paddr_t addr, paddr_t size)
> +{
> +    char *output, *input;
> +    unsigned char magic[2];
> +    int rc;
> +    unsigned kernel_order_in;
> +    unsigned kernel_order_out;
> +    paddr_t output_size;
> +    
> +    copy_from_paddr(magic, addr, sizeof(magic));

Need a size check before assuming sufficient length.  (All of the arm
probe functions suffer in a similar way.)

> +
> +    if (!((magic[0] == 0x1f) && ((magic[1] == 0x8b) || (magic[1] == 0x9e))))
> +        return -EINVAL;
> +
> +    kernel_order_in = get_order_from_bytes(size);
> +    input = (char *)ioremap_cache(addr, size);
> +
> +    output_size = output_length(input, size);
> +    kernel_order_out = get_order_from_bytes(output_size);
> +    output = (char *)alloc_xenheap_pages(kernel_order_out, 0);
> +
> +    rc = decompress(input, size, output);
> +    clean_dcache_va_range(output, output_size);
> +    iounmap(input);
> +
> +    if (rc != 0)

Xen style.

~Andrew

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

[Julien Grall]

On 12/08/15 17:35, Andrew Cooper wrote:
>> +
>> +static int kernel_zimage64_compressed_probe(struct kernel_info *info,
>> +                                 paddr_t addr, paddr_t size)
>> +{
>> +    char *output, *input;
>> +    unsigned char magic[2];
>> +    int rc;
>> +    unsigned kernel_order_in;
>> +    unsigned kernel_order_out;
>> +    paddr_t output_size;
>> +    
>> +    copy_from_paddr(magic, addr, sizeof(magic));
> 
> Need a size check before assuming sufficient length.  (All of the arm
> probe functions suffer in a similar way.)

Are you sure? kernel_zimage{32,64}_probe as long as kernel_uboot_probe
are checking the size before copying the magic...

Regards,

-- 
Julien Grall

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

[Jan Beulich]

>>> On 12.08.15 at 18:15, <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 12 Aug 2015, Jan Beulich wrote:
>> >>> On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> wrote:
>> > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
>> >   * dependent).
>> >   */
>> >  
>> > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
>> > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
>> >  
>> >  int decompress(void *inbuf, unsigned int len, void *outbuf);
>> >  
>> > +static inline unsigned long output_length(char *image, unsigned long image_len)
>> 
>> Neither of the callers gets moved out of bzimage.c - why does this
>> function need to move?
> 
> We'll use it on arm.

Hmm, the way it is used on x86 makes it quite architecture specific
(namely because of the assumption that the size is also in said
place for non-gz compression methods). I'd therefore prefer code
duplication over code sharing here. 

Jan

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

[Stefano Stabellini]

On Thu, 13 Aug 2015, Jan Beulich wrote:
> >>> On 12.08.15 at 18:15, <stefano.stabellini@eu.citrix.com> wrote:
> > On Wed, 12 Aug 2015, Jan Beulich wrote:
> >> >>> On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> wrote:
> >> > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
> >> >   * dependent).
> >> >   */
> >> >  
> >> > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
> >> > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
> >> >  
> >> >  int decompress(void *inbuf, unsigned int len, void *outbuf);
> >> >  
> >> > +static inline unsigned long output_length(char *image, unsigned long image_len)
> >> 
> >> Neither of the callers gets moved out of bzimage.c - why does this
> >> function need to move?
> > 
> > We'll use it on arm.
> 
> Hmm, the way it is used on x86 makes it quite architecture specific
> (namely because of the assumption that the size is also in said
> place for non-gz compression methods). I'd therefore prefer code
> duplication over code sharing here. 

Actually after seeing the size and quality of the resulting patches, I
am starting to feel the same way.

In terms of code changes, I was thinking that the best result would be
moving the "boilerplate" code from xen/arch/x86/bzimage.c to
xen/common/inflate.c, see below, then the interface would become just
perform_gunzip and gzip_check. But I guess you wouldn't want inflate.c
to be modified, right?

Alternatively we could move it to a new file, let's call it gunzip.h,
that would #include "inflate.c", so:

bzimage.c -- #include --> gunzip.h -- #include --> inflate.c

And again we just leave the perform_gunzip and gzip_check calls in
bzimage.c.  What do you think?

---


diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index c86c39e..cfd34d6 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -8,144 +8,8 @@
 #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/inflate.c b/xen/common/inflate.c
index f99c985..10c236a 100644
--- a/xen/common/inflate.c
+++ b/xen/common/inflate.c
@@ -103,6 +103,68 @@
       the two sets of lengths.
  */
 
+#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())
+
+#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;
+}
+
 #ifdef RCSID
 static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
 #endif
@@ -1302,3 +1364,54 @@ static int INIT gunzip(void)
     error("out of input data");
     return -1;
 }
+
+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 int perform_gunzip(char *output, char *image, unsigned long image_len)
+{
+    int rc;
+
+    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;
+}

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

[Jan Beulich]

>>> On 13.08.15 at 11:28, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 13 Aug 2015, Jan Beulich wrote:
>> >>> On 12.08.15 at 18:15, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Wed, 12 Aug 2015, Jan Beulich wrote:
>> >> >>> On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
>> >> >   * dependent).
>> >> >   */
>> >> >  
>> >> > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
>> >> > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, unlz4;
>> >> >  
>> >> >  int decompress(void *inbuf, unsigned int len, void *outbuf);
>> >> >  
>> >> > +static inline unsigned long output_length(char *image, unsigned long image_len)
>> >> 
>> >> Neither of the callers gets moved out of bzimage.c - why does this
>> >> function need to move?
>> > 
>> > We'll use it on arm.
>> 
>> Hmm, the way it is used on x86 makes it quite architecture specific
>> (namely because of the assumption that the size is also in said
>> place for non-gz compression methods). I'd therefore prefer code
>> duplication over code sharing here. 
> 
> Actually after seeing the size and quality of the resulting patches, I
> am starting to feel the same way.
> 
> In terms of code changes, I was thinking that the best result would be
> moving the "boilerplate" code from xen/arch/x86/bzimage.c to
> xen/common/inflate.c, see below, then the interface would become just
> perform_gunzip and gzip_check. But I guess you wouldn't want inflate.c
> to be modified, right?

Yes, unless really unavoidable.

> Alternatively we could move it to a new file, let's call it gunzip.h,
> that would #include "inflate.c", so:
> 
> bzimage.c -- #include --> gunzip.h -- #include --> inflate.c
> 
> And again we just leave the perform_gunzip and gzip_check calls in
> bzimage.c.  What do you think?

That's an option.

Jan

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

[Ian Campbell]

On Thu, 2015-08-13 at 03:57 -0600, Jan Beulich wrote:
> > 
> > > > On 13.08.15 at 11:28, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 13 Aug 2015, Jan Beulich wrote:
> > > > > > On 12.08.15 at 18:15, <stefano.stabellini@eu.citrix.com> wrote:
> > > > On Wed, 12 Aug 2015, Jan Beulich wrote:
> > > > > > > > On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> 
> > > > > > > > wrote:
> > > > > > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char 
> > > > > > *inbuf, unsigned int len,
> > > > > >   * dependent).
> > > > > >   */
> > > > > >  
> > > > > > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
> > > > > > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, 
> > > > > > unlz4;
> > > > > >  
> > > > > >  int decompress(void *inbuf, unsigned int len, void *outbuf);
> > > > > >  
> > > > > > +static inline unsigned long output_length(char *image, 
> > > > > > unsigned long image_len)
> > > > > 
> > > > > Neither of the callers gets moved out of bzimage.c - why does 
> > > > > this
> > > > > function need to move?
> > > > 
> > > > We'll use it on arm.
> > > 
> > > Hmm, the way it is used on x86 makes it quite architecture specific
> > > (namely because of the assumption that the size is also in said
> > > place for non-gz compression methods). I'd therefore prefer code
> > > duplication over code sharing here. 
> > 
> > Actually after seeing the size and quality of the resulting patches, I
> > am starting to feel the same way.
> > 
> > In terms of code changes, I was thinking that the best result would be
> > moving the "boilerplate" code from xen/arch/x86/bzimage.c to
> > xen/common/inflate.c, see below, then the interface would become just
> > perform_gunzip and gzip_check. But I guess you wouldn't want inflate.c
> > to be modified, right?
> 
> Yes, unless really unavoidable.
> 
> > Alternatively we could move it to a new file, let's call it gunzip.h,
> > that would #include "inflate.c", so:
> > 
> > bzimage.c -- #include --> gunzip.h -- #include --> inflate.c
> > 
> > And again we just leave the perform_gunzip and gzip_check calls in
> > bzimage.c.  What do you think?

How about putting perform_gunzip and gzip_check into a new gunzip.c which
includes inflate.c?

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

[Jan Beulich]

>>> On 13.08.15 at 12:17, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-08-13 at 03:57 -0600, Jan Beulich wrote:
>> > 
>> > > > On 13.08.15 at 11:28, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Thu, 13 Aug 2015, Jan Beulich wrote:
>> > > > > > On 12.08.15 at 18:15, <stefano.stabellini@eu.citrix.com> wrote:
>> > > > On Wed, 12 Aug 2015, Jan Beulich wrote:
>> > > > > > > > On 12.08.15 at 16:47, <stefano.stabellini@eu.citrix.com> 
>> > > > > > > > wrote:
>> > > > > > @@ -31,8 +33,15 @@ typedef int decompress_fn(unsigned char 
>> > > > > > *inbuf, unsigned int len,
>> > > > > >   * dependent).
>> > > > > >   */
>> > > > > >  
>> > > > > > -decompress_fn bunzip2, unxz, unlzma, unlzo, unlz4;
>> > > > > > +decompress_fn perform_gunzip, bunzip2, unxz, unlzma, unlzo, 
>> > > > > > unlz4;
>> > > > > >  
>> > > > > >  int decompress(void *inbuf, unsigned int len, void *outbuf);
>> > > > > >  
>> > > > > > +static inline unsigned long output_length(char *image, 
>> > > > > > unsigned long image_len)
>> > > > > 
>> > > > > Neither of the callers gets moved out of bzimage.c - why does 
>> > > > > this
>> > > > > function need to move?
>> > > > 
>> > > > We'll use it on arm.
>> > > 
>> > > Hmm, the way it is used on x86 makes it quite architecture specific
>> > > (namely because of the assumption that the size is also in said
>> > > place for non-gz compression methods). I'd therefore prefer code
>> > > duplication over code sharing here. 
>> > 
>> > Actually after seeing the size and quality of the resulting patches, I
>> > am starting to feel the same way.
>> > 
>> > In terms of code changes, I was thinking that the best result would be
>> > moving the "boilerplate" code from xen/arch/x86/bzimage.c to
>> > xen/common/inflate.c, see below, then the interface would become just
>> > perform_gunzip and gzip_check. But I guess you wouldn't want inflate.c
>> > to be modified, right?
>> 
>> Yes, unless really unavoidable.
>> 
>> > Alternatively we could move it to a new file, let's call it gunzip.h,
>> > that would #include "inflate.c", so:
>> > 
>> > bzimage.c -- #include --> gunzip.h -- #include --> inflate.c
>> > 
>> > And again we just leave the perform_gunzip and gzip_check calls in
>> > bzimage.c.  What do you think?
> 
> How about putting perform_gunzip and gzip_check into a new gunzip.c which
> includes inflate.c?

Would seem as good to me, provided specifically the output_length()
helper then can stay where it is.

Jan